Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove some ._filenames uses in favor of .filenames #12996

Merged
merged 2 commits into from
Dec 2, 2024
Merged

remove some ._filenames uses in favor of .filenames #12996

merged 2 commits into from
Dec 2, 2024

Conversation

sappelhoff
Copy link
Member

following #12843

I think it'd be good to reduce the use of private attributes in our own code base as well, if we might as well use the public attribute.

sappelhoff
sappelhoff commented Nov 28, 2024
View reviewed changes
mne/io/base.py Outdated Show resolved Hide resolved
@larsoner
Copy link
Member

I don't have time to look deeply yet, just a quick comment that in the PR where we made the switch to paths and a public attribute, at some point I used git grep and tried to change the ones that I thought made sense to change, and left ones behind that didn't make sense to change or couldn't be changed. It's entirely possible I missed a few or I was wrong about what couldn't be changed though!

@sappelhoff
Copy link
Member Author

@larsoner @cbrnr the fix in c40c6f4 seems to have solved the failing tests. I believe the changes here can truly be converted to use the public attribute.

@cbrnr
Copy link
Contributor

cbrnr commented Nov 29, 2024

That's great and 👍 for merging, but I am still a bit concerned why only raw._filenames and not raw.filenames works, as these two are supposed to point to the same object, or no?

@larsoner larsoner merged commit ec77e7c into mne-tools:main Dec 2, 2024
28 checks passed
@larsoner
Copy link
Member

larsoner commented Dec 2, 2024

Thanks @sappelhoff !

@sappelhoff sappelhoff deleted the filenames branch December 2, 2024 16:18
@sappelhoff
Copy link
Member Author

, but I am still a bit concerned why only raw._filenames and not raw.filenames works, as these two are supposed to point to the same object, or no?

See: #12996 (comment)

This is code in a small "helper class" that does not inherit anything from our other objects.

@cbrnr
Copy link
Contributor

cbrnr commented Dec 2, 2024

But why can it not use raw.filenames?

@sappelhoff
Copy link
Member Author

It probably can, if we make more code adjustments. Maybe worth a follow up in the future! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock Awaiting requested review from drammock drammock is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

@cbrnr cbrnr Awaiting requested review from cbrnr cbrnr is a code owner

@adam2392 adam2392 Awaiting requested review from adam2392 adam2392 is a code owner

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@rob-luke rob-luke Awaiting requested review from rob-luke rob-luke is a code owner

Assignees
No one assigned
Labels
no-changelog-entry-needed skip-changelog-checks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sappelhoff @larsoner @cbrnr